Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: swagger2 validator should only report error if there's body parameter but no consume #123

Merged
merged 4 commits into from
Jan 27, 2020
Merged

fix: swagger2 validator should only report error if there's body parameter but no consume #123

merged 4 commits into from
Jan 27, 2020

Conversation

SaltedCaramelCoffee
Copy link
Contributor

Currently, swagger2 validator reports error on all PUT or POST operations that has no consume field. This is technically incorrect because a POST operation is not required to have a request body and if there is no request body, there does not need to be a consumes field.

The fix now checks for body parameters on the path level and also operation level, and will only report error when there is body parameter, but no consume field.

@codecov
Copy link

codecov bot commented Jan 27, 2020

Codecov Report

Merging #123 into master will decrease coverage by 0.11%.
The diff coverage is 72.72%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #123      +/-   ##
==========================================
- Coverage   92.55%   92.43%   -0.12%     
==========================================
  Files          56       56              
  Lines        2027     2037      +10     
==========================================
+ Hits         1876     1883       +7     
- Misses        151      154       +3
Impacted Files Coverage Δ
...ion/swagger2/semantic-validators/operations-ibm.js 90.38% <72.72%> (-4.86%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c6b5510...fef4077. Read the comment docs.

Copy link
Member

@dpopp07 dpopp07 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple small changes to make, looks good otherwise

const checkStatus = config.no_consumes_for_put_or_post;

if (checkStatus !== 'off') {
result[checkStatus].push({
path: `paths.${pathKey}.${opKey}.consumes`,
message:
'PUT and POST operations must have a non-empty `consumes` field.'
'PUT and POST operations with body parameter must have a non-empty `consumes` field.'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a small wording suggestion. Might want to do this on line 2 as well

Suggested change
'PUT and POST operations with body parameter must have a non-empty `consumes` field.'
'PUT and POST operations with a body parameter must have a non-empty `consumes` field.'

@@ -84,7 +84,7 @@ describe('validation plugin - semantic - operations-ibm - swagger2', function()
expect(res.errors.length).toEqual(1);
expect(res.errors[0].path).toEqual('paths./CoolPath.post.consumes');
expect(res.errors[0].message).toEqual(
'PUT and POST operations must have a non-empty `consumes` field.'
'PUT and POST operations with body parameter must have a non-empty `consumes` field.'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will you also add a test exercising your changes? As in, a test where there is no consumes and no body parameter and show that there are no errors

'/CoolPath': {
parameters: [
{
name: 'BadParameter',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Poor parameter - there's something wrong with the operation, not him 😉

Copy link
Member

@dpopp07 dpopp07 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! 👍

@SaltedCaramelCoffee SaltedCaramelCoffee merged commit 1d97cb2 into IBM:master Jan 27, 2020
@SaltedCaramelCoffee SaltedCaramelCoffee deleted the post-ops-without-request-body branch January 27, 2020 20:20
dpopp07 pushed a commit that referenced this pull request Jan 27, 2020
## [0.16.2](v0.16.1...v0.16.2) (2020-01-27)

### Bug Fixes

* swagger2 validator should only report error if there's body parameter but no consume ([#123](#123)) ([1d97cb2](1d97cb2))
@dpopp07
Copy link
Member

dpopp07 commented Jan 27, 2020

🎉 This PR is included in version 0.16.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants